Skip to content

improve: deprecate instance based Mappers.fromOwnerReferences#3365

Merged
csviri merged 2 commits into
operator-framework:mainfrom
csviri:deprecate-from-resource
May 20, 2026
Merged

improve: deprecate instance based Mappers.fromOwnerReferences#3365
csviri merged 2 commits into
operator-framework:mainfrom
csviri:deprecate-from-resource

Conversation

@csviri
Copy link
Copy Markdown
Collaborator

@csviri csviri commented May 19, 2026

This does not make sense to me, this should be always based on class

Signed-off-by: Attila Mészáros a_meszaros@apple.com

Copilot AI review requested due to automatic review settings May 19, 2026 20:15
@openshift-ci openshift-ci Bot requested review from metacosm and xstefank May 19, 2026 20:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR deprecates the instance-based Mappers.fromOwnerReferences(HasMetadata ...) overloads in favor of using class-based (or explicit apiVersion/kind) inputs, aligning mapper creation with static primary resource types.

Changes:

  • Mark fromOwnerReferences(HasMetadata) as deprecated for removal.
  • Mark fromOwnerReferences(HasMetadata, boolean) as deprecated for removal.
Comments suppressed due to low confidence (1)

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java:101

  • Same here: the deprecation annotation doesn’t indicate the intended replacement. Please add a Javadoc @deprecated message (and optionally an inline @link) so consumers know to switch to the class-based overload.
  @Deprecated(forRemoval = true)
  public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerReferences(
      HasMetadata primaryResource, boolean clusterScoped) {
    return fromOwnerReferences(
        primaryResource.getApiVersion(), primaryResource.getKind(), clusterScoped);
  }

Comment on lines 89 to 95

@Deprecated(forRemoval = true)
public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerReferences(
HasMetadata primaryResource) {
return fromOwnerReferences(primaryResource, false);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I also think that all deprecations should point to replacements or explain why the method is being dropped.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, added javadoc to the alternative

Comment on lines 89 to 95

@Deprecated(forRemoval = true)
public static <T extends HasMetadata> SecondaryToPrimaryMapper<T> fromOwnerReferences(
HasMetadata primaryResource) {
return fromOwnerReferences(primaryResource, false);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I also think that all deprecations should point to replacements or explain why the method is being dropped.

csviri added 2 commits May 20, 2026 14:08
This does not make sense to me, this should be always based on class

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri force-pushed the deprecate-from-resource branch from 0e8de49 to 8e48dbb Compare May 20, 2026 12:08
@csviri csviri requested a review from Copilot May 20, 2026 12:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@csviri csviri merged commit 1519be9 into operator-framework:main May 20, 2026
27 checks passed
@csviri csviri deleted the deprecate-from-resource branch May 20, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants